Skip to content

Conversation

@NetworkOverload
Copy link

I think this PR is best explained by an example. I do not think the code is good enough to actually accept the PR at this time, but I want feedback on whether this kind of modification is reasonable or could be accepted if polished.

SERVICE = "service-name"

@rpc()
def setup(root, log, **kwargs):
    rootcfg = root.get_root()
    rootcfg.set_creator(f"{SERVICE}[service-name='{kwargs['instance']}']")
    rootcfg = functionA(rootcfg)
    return root
    
    
# Main only gets called when debugging from shell
def main():
    clx = Clixon(source="running",target="candidate")
    rootcfg = clx.get_root()
    rootcfg.set_creator(f"{SERVICE}[service-name='mesh1']")
    functionA(rootcfg)
	
	
def functionA(config):
	config_branch = get_path(config,"config_branch")
	functionB(config_branch)

def functionB(config_branch):
	new_key = config_branch.create('new-key')
	
	# At this point multiple-function calls in, we don't have the context of the instance anymore, 
	# so it either requres passing it through every function call, defining global vars, 
	# or just using a class variable to store that context so it can be retrieved by a new method.
	new_key.tag_creator()

if __name__ == "__main__":
    main()

matching_children = [x for x in self._children if x._name == key]
if matching_children:
if len(matching_children) == 1:
self.__dict__[key] = matching_children[0]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already removed in another commit, can we exclude it from this one?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed from the commit.

Added class variable to store a creator tag so that the tag can be
applied easily in scopes that do not have access to the service instance.
@krihal
Copy link
Collaborator

krihal commented Nov 21, 2024

@JohannesGarm can you look at this since you've requested something similar?

@JohannesGarm
Copy link
Contributor

This is close to the solution we discussed and a definite improvement to the current system. I see why you have to have the service itself set the creator variable but a mandatory call which is the same across all services is still counter-intuitive to me. I think it would be cleaner to have the formating for the string done by the set_creator function - I cant think of a situation where you can set it to any other string format and it would not just break.

Does this work when editing service config from the service code? If you create a leaf under root.services.properties, for instance?

@NetworkOverload
Copy link
Author

Yesterday when I fixed the PR the first thing I realized was it would probably be better to have the set_creator method do the string formatting to make it cleaner. However, when I wrote it, I didn't fully understand what kinds of other attributes a person might want to apply when making this call. I figured this was a way to not box anyone out, they can feed any data in they want in case I overlooked something. I originally didn't expect to commit it, I thought I could find a way to augment the class at runtime later, but turns out that is harder than I thought.

I didn't check if it works for editing the service config, I've been using it for editing remote system configs. However, since it is a global variable in the Element class, as long as the tag is valid, it will work anywhere you are editing XML.

I will look at the docs again with regards to attributes and then look at the formatting again, it may be a more proper solution to write something that can understand all of the XML properties required by the service API and allows a user to selectively apply them, possibly allow setting the default tag for the method if no arguments are passed or something similar to keep the simple approach.

@krihal
Copy link
Collaborator

krihal commented Nov 21, 2024

Please add unit tests covering the added functionality.

@JohannesGarm
Copy link
Contributor

I think the approach to make it a function to apply all the attributes could be good. At present, these are the attributes used in my service code:

  • "cl:creator": As the ticket already discusses,
  • "nc:operation": replace/merge strategy for config application,
  • "xmlns:cl": clixon xmlns - I think this is mostly static,
  • "xmlns": Vendor xmlns - this one is tricky and will introduce complications once we have multivendor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants